Skip to content

Enhance audit logging: add event name and description, refine mutatio…#74

Merged
mattknatt merged 7 commits into
mainfrom
feat/auditcleanup
Apr 22, 2026
Merged

Enhance audit logging: add event name and description, refine mutatio…#74
mattknatt merged 7 commits into
mainfrom
feat/auditcleanup

Conversation

@mattknatt

@mattknatt mattknatt commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

…n tracking, and update UI to display detailed audit event information.

Summary by CodeRabbit

  • New Features

    • Live audit stream endpoint for managers with server-sent events, connection status, and dashboard indicator
    • Audit events include explicit event names and human‑readable descriptions
  • Bug Fixes

    • Auditing limited to mutation requests to reduce noise
    • API unauthenticated responses now return 401; standardized 400/404 error handling
  • UI Improvements

    • Redesigned audit table (Event, Description, Details), standardized timestamps, actor consolidation, truncated case links, live row highlights
  • Tests/Chores

    • Tests updated to exercise mutation requests; CSRF eager initialization added

…n tracking, and update UI to display detailed audit event information.
@coderabbitai

coderabbitai Bot commented Apr 21, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@mattknatt has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 49 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 49 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f300e267-fc6c-4b65-81f9-723bd9eee7ba

📥 Commits

Reviewing files that changed from the base of the PR and between 41d8a44 and 6e824ee.

📒 Files selected for processing (4)
  • src/main/java/org/example/projektarendehantering/application/service/AuditService.java
  • src/main/resources/static/app.css
  • src/main/resources/templates/audit/list.html
  • src/main/resources/templates/fragments/head.html
📝 Walkthrough

Walkthrough

Replaces persisted HTTP method with semantic eventName and description, timestamps audit events, restricts web auditing to mutation requests, adds SSE streaming with manager-only /api/audit/stream, updates UI to stream and show event/description/details, adds CSRF eager init interceptor and targeted controller exception handlers.

Changes

Cohort / File(s) Summary
Audit Model & Mapping
src/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventEntity.java, src/main/java/org/example/projektarendehantering/presentation/dto/AuditEventDTO.java, src/main/java/org/example/projektarendehantering/application/service/AuditEventMapper.java
Removed httpMethod; added eventName and description; mapper updated to populate new fields and omit HTTP method.
Web Interceptor & SSE
src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java, src/main/java/org/example/projektarendehantering/application/service/AuditService.java, src/main/java/org/example/projektarendehantering/presentation/rest/AuditController.java
Interceptor skips non-mutation requests (unless error), builds WEB_ACTION events with description "<METHOD> <URI>"; AuditService persists after commit and broadcasts to active SseEmitters; added GET /api/audit/stream producing text/event-stream requiring manager role.
Domain Services Audit Enrichment
src/main/java/org/example/projektarendehantering/application/service/CaseService.java, src/main/java/org/example/projektarendehantering/application/service/DocumentService.java
Case/Document audit entries now include eventName (e.g., CASE_CREATED, STATUS_CHANGED, DOCUMENT_UPLOADED), human-readable description, and occurredAt timestamps.
Presentation / UI
src/main/resources/templates/audit/list.html, src/main/resources/templates/fragments/header.html
Audit table refactored to event-focused columns (Event, Description, Details), time formatting, case links and badges; header adds SSE client logic and visual audit indicator for managers; client-side streaming inserts new rows.
Error Handling
src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java
Added handlers for NoResourceFoundException, AsyncRequestNotUsableException, and consolidated 400 handling for unreadable/validation errors with REST vs non-REST branching.
CSRF / MVC Config
src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.java, src/main/java/org/example/projektarendehantering/infrastructure/web/CsrfEagerInitInterceptor.java
Registered new CsrfEagerInitInterceptor before AuditInterceptor; new interceptor forces CSRF token initialization in preHandle.
Security
src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java
Added exceptionHandling entry point to return 401 for /api/ requests.
Tests
src/test/.../AuditIntegrationTest.java, src/test/.../ProjektArendehanteringApplicationTests.java, src/test/.../presentation/rest/AuditControllerTest.java, src/test/.../presentation/rest/CaseControllerTest.java
Tests adjusted to use CSRF-protected mutation requests, expect 401 for unauthenticated API calls, and removed assertions depending on persisted HTTP method.
Docs
AUDIT.md
Documentation updated to remove httpMethod from schema and reflect new recorded metadata and UI column names.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Interceptor as AuditInterceptor
    participant Domain as DomainService
    participant AuditSvc as AuditService
    participant DB as AuditEventEntity (Repository/DB)
    rect rgba(100,150,200,0.5)
        Note over Client,Interceptor: Incoming web request
        Client->>Interceptor: HTTP Request (method, uri)
        Interceptor->>Interceptor: Is mutation? (POST/PUT/DELETE/PATCH) or error present
        alt Mutation or error
            Interceptor->>AuditSvc: Build AuditEvent (eventName="WEB_ACTION", description="<METHOD> <URI>", path..., occurredAt=now)
            AuditSvc->>DB: Persist audit event (after transaction commit)
            AuditSvc->>AuditSvc: Broadcast DTO -> active SseEmitters
        else Non-mutation without error
            Interceptor-->>Interceptor: Skip audit
        end
    end

    rect rgba(150,200,100,0.5)
        Note over Client,Domain: Domain-triggered audit
        Client->>Domain: Domain operation (create/update/upload)
        Domain->>Domain: Business logic
        Domain->>AuditSvc: Build AuditEvent (eventName e.g., "CASE_CREATED", description, occurredAt=now)
        AuditSvc->>DB: Persist audit event (after transaction commit)
        AuditSvc->>AuditSvc: Broadcast DTO -> active SseEmitters
    end

    rect rgba(200,150,180,0.5)
        Note over Client,AuditSvc: SSE consumer (UI)
        Client->>AuditSvc: GET /api/audit/stream (requires manager)
        AuditSvc-->>Client: SseEmitter stream ("connected", "audit-event"...)
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • OskarLundqvist33

Poem

🐰 I nibble logs and stitch each event bright,
I hop when WEB_ACTION and CASE_CREATED take flight,
Streams whisper to managers with badges in sight,
New names, clear descriptions, timestamps at night,
Hooray — my audit meadow hums with delight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the main changes: adding event name and description to audit logging, refining mutation tracking. The truncation ('mutation…') is a minor formatting issue but does not detract from clarity.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/auditcleanup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
AUDIT.md (2)

33-51: ⚠️ Potential issue | 🟡 Minor

Data model table is missing the new eventName and description fields.

The PR removed httpMethod from the table but did not add rows for the newly persisted eventName and description columns of AuditEventEntity. The documentation now under-describes the schema.

 | `statusChange` | String | ... |
+| `eventName` | String | High-level event identifier (e.g., `WEB_ACTION`, `CASE_CREATED`, `STATUS_CHANGED`, `DOCUMENT_UPLOADED`) |
+| `description` | String | Human-readable description of the event |
 | `clientIp` | String | `X-Forwarded-For` first hop, else `remoteAddr` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AUDIT.md` around lines 33 - 51, The audit_events schema doc was not updated
to include the new fields from AuditEventEntity: add rows for eventName (String
- logical event identifier) and description (String - human-readable event
description) to the table definition, and remove the obsolete httpMethod row if
still present; ensure the documentation mentions these fields are persisted by
the interceptor (matching AuditEventEntity.eventName and
AuditEventEntity.description) and update the Indexes/notes if either field
requires indexing or sanitization like queryString.

193-196: ⚠️ Potential issue | 🟡 Minor

Test description is out of date.

The referenced test now performs POST /ui/cases/new (a mutation), not GET /ui/cases. Also, the new audit policy only audits mutation requests, which is the entire point of the test change — worth reflecting here.

-`ProjektArendehanteringApplicationTests.uiRequest_createsAuditEvent` asserts that an authenticated `GET /ui/cases` increases `AuditEventRepository.count()`, verifying the interceptor → `record` path.
+`ProjektArendehanteringApplicationTests.uiRequest_createsAuditEvent` asserts that an authenticated `POST /ui/cases/new` (a mutation) increases `AuditEventRepository.count()`, verifying the interceptor → `record` path and the mutation-only audit policy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AUDIT.md` around lines 193 - 196, Update the Tests section to reflect that
ProjektArendehanteringApplicationTests.uiRequest_createsAuditEvent now issues a
POST to /ui/cases/new (a mutation) rather than GET /ui/cases, and state that the
current audit policy audits mutation requests only; specifically mention that
the test asserts an authenticated POST /ui/cases/new increases
AuditEventRepository.count(), verifying the interceptor → record path under the
mutation-only audit policy.
src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (1)

99-128: ⚠️ Potential issue | 🟠 Major

Audit recording inside the compensation try — verify intended semantics.

auditService.record(...) is invoked inside the try block that triggers S3 cleanup on RuntimeException. If the audit save itself throws (e.g., DB constraint once description length is bounded, see sibling comment), the outer catch will clean up the S3 object and rethrow, which means an audit failure can now cascade into failing the user's upload. Per AUDIT.md line 92, audit failures are supposed to never break the user’s request. Consider wrapping auditService.record(...) in its own try/catch here as well, mirroring the interceptor contract.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`
around lines 99 - 128, The auditService.record(...) calls inside
DocumentService's save/compensation try block must be isolated so audit failures
don't trigger the outer RuntimeException path and S3 cleanup; wrap each
auditService.record(...) invocation in its own try/catch that catches Exception
(or RuntimeException), logs the audit failure (with context: caseId, eventName,
description, actorId) and swallows the exception (do not rethrow) so the user's
document upload succeeds even if audit persistence fails.
🧹 Nitpick comments (8)
src/main/java/org/example/projektarendehantering/application/service/CaseService.java (1)

328-343: occurredAt is now set twice — benign but redundant.

AuditService.record() already defaults occurredAt to Instant.now() when null (documented in AUDIT.md line 89). Setting it here is harmless but creates two sources of truth; if someone later adds server-clock-sensitive logic in record() (e.g., clock skew handling), the pre-set value will silently win. Consider either leaving it to record() or — preferably — making record() authoritative and dropping it from the builders in both CaseService and DocumentService.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`
around lines 328 - 343, Remove the redundant occurredAt assignment in the
AuditEventEntity builder inside recordStatusChange so auditService.record(...)
can set the timestamp authoritatively; specifically, edit the recordStatusChange
method to stop calling .occurredAt(Instant.now()) on the AuditEventEntity
builder (and make the same change in DocumentService where AuditEventEntity is
built) so that AuditService.record() is the single place that populates
occurredAt.
src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java (2)

112-133: Validation error details are dropped.

For MethodArgumentNotValidException, the generic "Validation failed" message hides the actual field errors, which makes it difficult for API clients (and the user-facing error view) to know what to fix. Consider surfacing field-level errors from getBindingResult().getFieldErrors() for REST requests (e.g., include a fieldErrors list in the body). Same for HttpMessageNotReadableException's most-specific cause when safe to expose.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`
around lines 112 - 133, In handleBadRequestException in GlobalControllerAdvice,
don't return only the generic "Validation failed": when e is a
MethodArgumentNotValidException extract field errors via
((MethodArgumentNotValidException)e).getBindingResult().getFieldErrors() and
include a serializable fieldErrors list (field, rejectedValue, defaultMessage)
in the REST ResponseEntity body and also add it to the Model for non-REST views;
for HttpMessageNotReadableException include the most-specific safe cause message
(e.getMostSpecificCause().getMessage()) in the response as an additional detail
property (only for REST or sanitized for views) so clients receive actionable
error details.

106-110: 404 handler bypasses REST/HTML response conventions.

Unlike the other handlers in this class, handleNoResourceFoundException returns void with @ResponseStatus, so Spring falls back to its default error dispatch (e.g. BasicErrorController) instead of producing the JSON body {errorCode, message, status} used by /api/** callers or the error view used elsewhere. If REST clients rely on the consistent error envelope, a missing API path will now return a differently-shaped response than e.g. AccessDeniedException or the new handleBadRequestException.

Consider mirroring the REST/HTML branching used in the neighboring handlers if consistency matters:

♻️ Suggested shape
-    `@ExceptionHandler`(NoResourceFoundException.class)
-    `@ResponseStatus`(HttpStatus.NOT_FOUND)
-    public void handleNoResourceFoundException(NoResourceFoundException e) {
-        // Just return 404, don't log as ERROR
-    }
+    `@ExceptionHandler`(NoResourceFoundException.class)
+    public Object handleNoResourceFoundException(NoResourceFoundException e,
+                                                 HttpServletRequest request,
+                                                 HttpServletResponse response,
+                                                 Model model) {
+        HttpStatus status = HttpStatus.NOT_FOUND;
+        if (isRestRequest(request)) {
+            return ResponseEntity.status(status).body(Map.of(
+                    "errorCode", "NOT_FOUND",
+                    "message", "Resource not found",
+                    "status", status.value()));
+        }
+        response.setStatus(status.value());
+        model.addAttribute("status", status.value() + " " + status.getReasonPhrase());
+        model.addAttribute("message", "Resource not found");
+        model.addAttribute("errorCode", "NOT_FOUND");
+        return "error";
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`
around lines 106 - 110, The NoResourceFoundException handler currently returns
void with `@ResponseStatus` which triggers Spring's default error dispatch; update
handleNoResourceFoundException(NoResourceFoundException e) to mirror the
neighboring handlers' REST/HTML branching: accept HttpServletRequest (and
optionally HttpServletResponse), detect API requests (e.g.
request.getServletPath().startsWith("/api/") or check Accept header), and for
API callers return the same JSON error envelope used elsewhere (errorCode,
message, status) as a ResponseEntity, while for non-API callers return the same
error view (ModelAndView "error") used by the other handlers so responses are
consistent across clients. Ensure you reuse the same error envelope construction
logic and view name used by the other handlers in this class.
src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java (1)

40-46: Brittle assumption about 200 OK for missing body.

The comment says the endpoint returns 200 because the body is missing (re-renders the form). This is a behavioral coupling — if /ui/cases/new ever validates CSRF + empty body and redirects/returns 4xx, the test will fail for reasons unrelated to auditing. Since the intent here is only to assert that a mutation produces an audit row, consider asserting status().is2xxSuccessful() (or a broader matcher) so the test remains focused on the audit count delta.

-        mockMvc.perform(post("/ui/cases/new").with(csrf()))
-                .andExpect(status().isOk()); // Returns 200 (re-renders form) because body is missing
+        mockMvc.perform(post("/ui/cases/new").with(csrf()));
+        // Status intentionally not asserted; this test only verifies that
+        // mutation requests produce an audit event.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java`
around lines 40 - 46, The test assumes a 200 OK for
mockMvc.perform(post("/ui/cases/new").with(csrf())) which is brittle; change the
status assertion to a broader 2xx check (e.g., use status().is2xxSuccessful())
so the test remains focused on the auditEventRepository.count() delta and not
the exact response behavior; update the assertion around mockMvc.perform(...)
and keep the subsequent assertThat(after).isGreaterThan(before) as-is.
AUDIT.md (1)

70-82: Interceptor description no longer mentions eventName/description.

Step 3 was updated to drop HTTP method, but the interceptor now also sets eventName = "WEB_ACTION" and description = "<method> <requestURI>". Adding a step (or note) here would keep the workflow description accurate and discoverable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AUDIT.md` around lines 70 - 82, The audit doc's "Building the event" steps
omit that the interceptor now sets eventName and description; update the section
(near the list of fields like actorId/principalName and the step about request
metadata) to state that the interceptor sets eventName = "WEB_ACTION" and
description = "<method> <requestURI>" (i.e., HTTP method followed by the request
URI), so the documentation reflects the actual behavior of the interceptor that
also constructs these two fields.
src/test/java/org/example/projektarendehantering/AuditIntegrationTest.java (1)

60-68: Consider asserting the new eventName and description fields.

The PR introduces eventName ("WEB_ACTION") and description (" ") on audit events, but the integration test no longer asserts either. Adding lightweight assertions (e.g., assertThat(latest.getEventName()).isEqualTo("WEB_ACTION"); and assertThat(latest.getDescription()).contains("POST").contains("/api/cases");) would lock in the new contract and catch regressions in the interceptor.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/projektarendehantering/AuditIntegrationTest.java`
around lines 60 - 68, AuditIntegrationTest currently checks the persisted
AuditEventEntity via auditEventRepository but misses assertions for the new
fields; update the test to assert the new eventName and description contract by
adding assertions on the retrieved AuditEventEntity (latest) such as asserting
latest.getEventName().equals("WEB_ACTION") and that latest.getDescription()
contains the HTTP method and path (e.g., contains("POST") and
contains("/api/cases")), keeping the existing requestPath assertion via
latest.getRequestPath() to validate the interceptor behavior.
src/main/resources/templates/audit/list.html (1)

65-65: #strings.substring on a UUID relies on implicit toString().

e.caseId is likely a UUID, not a String. Thymeleaf's #strings.substring generally coerces via toString(), but being explicit avoids surprises and is more robust. Also consider using ${#strings.abbreviate(e.caseId, 11)} or similar for the '...' affordance.

Proposed fix
-                    <a th:if="${e.caseId != null}" th:href="@{/ui/cases/{id}(id=${e.caseId})}" th:text="${`#strings.substring`(e.caseId,0,8)} + '...'">case</a>
+                    <a th:if="${e.caseId != null}" th:href="@{/ui/cases/{id}(id=${e.caseId})}" th:text="${`#strings.substring`(e.caseId.toString(),0,8) + '...'}">case</a>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/templates/audit/list.html` at line 65, The template
currently calls `#strings.substring` on e.caseId which may be a UUID; explicitly
convert e.caseId to string (e.g., use e.caseId.toString()) before substring to
avoid implicit coercion, or replace the expression with Thymeleaf's
`#strings.abbreviate`(e.caseId.toString(), 11) to produce a safe 8-char + '...'
display; update the expression that references e.caseId and the
`#strings.substring/`#strings.abbreviate call accordingly.
src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java (1)

60-61: Description duplicates fields already captured; consider sanitizing path tokens.

description is "<method> <requestURI>", while requestPath already stores the URI and the HTTP method is implicit in the mutation filter. If the goal is a human-readable summary, consider a more meaningful value (e.g., handler-derived action). Additionally, requestURI can embed sensitive path variables (tokens, emails); AuditService only sanitizes queryString, so such values will land unsanitized in description. Worth confirming this is acceptable for your audit retention/PII posture.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java`
around lines 60 - 61, The description currently duplicates requestPath and may
leak sensitive path variables: update the AuditInterceptor so
event.setDescription(...) does not include raw request.getRequestURI(); instead
derive a concise human-readable action (e.g., handler or controller method name)
or sanitize path tokens before setting description; reference the
event.setDescription call and request.getRequestURI usage and either replace it
with a handler-derived value (use the handler/controller info available in the
interceptor) or run the URI through the same sanitization used for queryString
in AuditService so requestPath and description remain safe and non-duplicative.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 118-127: The audit entry writes the raw uploaded filename into
AuditEventEntity.description via the DocumentService call to
auditService.record(...) using entity.getFileName(), which risks leaking PII and
unbounded length; update DocumentService to sanitize/redact and bound the
filename before building the AuditEventEntity (e.g., replace sensitive segments
or mask characters and/or truncate to a safe max length) and ensure
AuditEventEntity.description has a matching column length constraint so the
stored description cannot exceed DB limits; reference the builder usage around
AuditEventEntity.builder() and the entity.getFileName() call when applying these
changes.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java`:
- Around line 31-40: The current guard in afterCompletion uses isMutation and ex
to skip auditing non-mutations, which misses non-mutating requests that failed
without throwing; update the condition in AuditInterceptor.afterCompletion to
also consider HTTP response status by checking HttpServletResponse.getStatus()
(e.g., treat status >= 400 as an error) so that when ex == null but
response.getStatus() >= 400 you still proceed with audit logging; refer to the
isMutation boolean, the ex parameter and response.getStatus() when modifying the
early-return logic.

In `@src/main/resources/templates/audit/list.html`:
- Line 51: Replace the fragile expression that directly calls
`#temporals.format`(e.occurredAt, ...) with a null-safe, zone-explicit expression
in the template cell: guard e.occurredAt (e.g. use a ternary or th:if) so a null
occurredAt renders a safe placeholder like "—" instead of throwing, and
convert/format the timestamp in UTC (for example by converting to a
ZonedDateTime with T(java.time.ZoneId).of('UTC') before formatting) so the audit
time is not rendered using the JVM default zone; update the expression that
references e.occurredAt in list.html accordingly.

---

Outside diff comments:
In `@AUDIT.md`:
- Around line 33-51: The audit_events schema doc was not updated to include the
new fields from AuditEventEntity: add rows for eventName (String - logical event
identifier) and description (String - human-readable event description) to the
table definition, and remove the obsolete httpMethod row if still present;
ensure the documentation mentions these fields are persisted by the interceptor
(matching AuditEventEntity.eventName and AuditEventEntity.description) and
update the Indexes/notes if either field requires indexing or sanitization like
queryString.
- Around line 193-196: Update the Tests section to reflect that
ProjektArendehanteringApplicationTests.uiRequest_createsAuditEvent now issues a
POST to /ui/cases/new (a mutation) rather than GET /ui/cases, and state that the
current audit policy audits mutation requests only; specifically mention that
the test asserts an authenticated POST /ui/cases/new increases
AuditEventRepository.count(), verifying the interceptor → record path under the
mutation-only audit policy.

In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 99-128: The auditService.record(...) calls inside
DocumentService's save/compensation try block must be isolated so audit failures
don't trigger the outer RuntimeException path and S3 cleanup; wrap each
auditService.record(...) invocation in its own try/catch that catches Exception
(or RuntimeException), logs the audit failure (with context: caseId, eventName,
description, actorId) and swallows the exception (do not rethrow) so the user's
document upload succeeds even if audit persistence fails.

---

Nitpick comments:
In `@AUDIT.md`:
- Around line 70-82: The audit doc's "Building the event" steps omit that the
interceptor now sets eventName and description; update the section (near the
list of fields like actorId/principalName and the step about request metadata)
to state that the interceptor sets eventName = "WEB_ACTION" and description =
"<method> <requestURI>" (i.e., HTTP method followed by the request URI), so the
documentation reflects the actual behavior of the interceptor that also
constructs these two fields.

In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 328-343: Remove the redundant occurredAt assignment in the
AuditEventEntity builder inside recordStatusChange so auditService.record(...)
can set the timestamp authoritatively; specifically, edit the recordStatusChange
method to stop calling .occurredAt(Instant.now()) on the AuditEventEntity
builder (and make the same change in DocumentService where AuditEventEntity is
built) so that AuditService.record() is the single place that populates
occurredAt.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java`:
- Around line 60-61: The description currently duplicates requestPath and may
leak sensitive path variables: update the AuditInterceptor so
event.setDescription(...) does not include raw request.getRequestURI(); instead
derive a concise human-readable action (e.g., handler or controller method name)
or sanitize path tokens before setting description; reference the
event.setDescription call and request.getRequestURI usage and either replace it
with a handler-derived value (use the handler/controller info available in the
interceptor) or run the URI through the same sanitization used for queryString
in AuditService so requestPath and description remain safe and non-duplicative.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 112-133: In handleBadRequestException in GlobalControllerAdvice,
don't return only the generic "Validation failed": when e is a
MethodArgumentNotValidException extract field errors via
((MethodArgumentNotValidException)e).getBindingResult().getFieldErrors() and
include a serializable fieldErrors list (field, rejectedValue, defaultMessage)
in the REST ResponseEntity body and also add it to the Model for non-REST views;
for HttpMessageNotReadableException include the most-specific safe cause message
(e.getMostSpecificCause().getMessage()) in the response as an additional detail
property (only for REST or sanitized for views) so clients receive actionable
error details.
- Around line 106-110: The NoResourceFoundException handler currently returns
void with `@ResponseStatus` which triggers Spring's default error dispatch; update
handleNoResourceFoundException(NoResourceFoundException e) to mirror the
neighboring handlers' REST/HTML branching: accept HttpServletRequest (and
optionally HttpServletResponse), detect API requests (e.g.
request.getServletPath().startsWith("/api/") or check Accept header), and for
API callers return the same JSON error envelope used elsewhere (errorCode,
message, status) as a ResponseEntity, while for non-API callers return the same
error view (ModelAndView "error") used by the other handlers so responses are
consistent across clients. Ensure you reuse the same error envelope construction
logic and view name used by the other handlers in this class.

In `@src/main/resources/templates/audit/list.html`:
- Line 65: The template currently calls `#strings.substring` on e.caseId which may
be a UUID; explicitly convert e.caseId to string (e.g., use e.caseId.toString())
before substring to avoid implicit coercion, or replace the expression with
Thymeleaf's `#strings.abbreviate`(e.caseId.toString(), 11) to produce a safe
8-char + '...' display; update the expression that references e.caseId and the
`#strings.substring/`#strings.abbreviate call accordingly.

In `@src/test/java/org/example/projektarendehantering/AuditIntegrationTest.java`:
- Around line 60-68: AuditIntegrationTest currently checks the persisted
AuditEventEntity via auditEventRepository but misses assertions for the new
fields; update the test to assert the new eventName and description contract by
adding assertions on the retrieved AuditEventEntity (latest) such as asserting
latest.getEventName().equals("WEB_ACTION") and that latest.getDescription()
contains the HTTP method and path (e.g., contains("POST") and
contains("/api/cases")), keeping the existing requestPath assertion via
latest.getRequestPath() to validate the interceptor behavior.

In
`@src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java`:
- Around line 40-46: The test assumes a 200 OK for
mockMvc.perform(post("/ui/cases/new").with(csrf())) which is brittle; change the
status assertion to a broader 2xx check (e.g., use status().is2xxSuccessful())
so the test remains focused on the auditEventRepository.count() delta and not
the exact response behavior; update the assertion around mockMvc.perform(...)
and keep the subsequent assertThat(after).isGreaterThan(before) as-is.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a4136a6f-edd6-486a-8074-6d8242a18a6d

📥 Commits

Reviewing files that changed from the base of the PR and between 46b18db and e5c85bb.

📒 Files selected for processing (12)
  • AUDIT.md
  • src/main/java/org/example/projektarendehantering/application/service/AuditEventMapper.java
  • src/main/java/org/example/projektarendehantering/application/service/CaseService.java
  • src/main/java/org/example/projektarendehantering/application/service/DocumentService.java
  • src/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventEntity.java
  • src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java
  • src/main/java/org/example/projektarendehantering/presentation/dto/AuditEventDTO.java
  • src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java
  • src/main/resources/templates/audit/list.html
  • src/test/java/org/example/projektarendehantering/AuditIntegrationTest.java
  • src/test/java/org/example/projektarendehantering/ProjektArendehanteringApplicationTests.java
  • src/test/java/org/example/projektarendehantering/presentation/rest/AuditControllerTest.java
💤 Files with no reviewable changes (1)
  • src/test/java/org/example/projektarendehantering/presentation/rest/AuditControllerTest.java

Comment on lines +118 to 127
} else {
auditService.record(AuditEventEntity.builder()
.caseId(caseEntity.getId())
.eventName("DOCUMENT_UPLOADED")
.description("Document uploaded: " + entity.getFileName())
.actorId(actor.userId())
.actorRole(actor.role() != null ? actor.role().name() : null)
.occurredAt(Instant.now())
.build());
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

DOCUMENT_UPLOADED audit entry includes raw filename — consider length/PII handling.

entity.getFileName() comes directly from the uploaded MultipartFile.getOriginalFilename() and is written verbatim into description. Since:

  • Filenames can legitimately contain PII or sensitive content (e.g., patient-1234-ssn.pdf), and
  • The queryString sanitization path in AuditService.record() (per AUDIT.md) only scrubs the queryString field, leaving description untouched,

this can leak sensitive data into the audit table and onto the audit UI. Consider either truncating/escaping the filename, or documenting that audit description is a non-sanitized field so operators can reason about it.

Additionally, no upper bound on description length means a pathologically long filename could exceed the default column length once a DDL is generated with a length constraint. A @Column(length = ...) on AuditEventEntity.description with matching truncation here would make behavior predictable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`
around lines 118 - 127, The audit entry writes the raw uploaded filename into
AuditEventEntity.description via the DocumentService call to
auditService.record(...) using entity.getFileName(), which risks leaking PII and
unbounded length; update DocumentService to sanitize/redact and bound the
filename before building the AuditEventEntity (e.g., replace sensitive segments
or mask characters and/or truncate to a safe max length) and ensure
AuditEventEntity.description has a matching column length constraint so the
stored description cannot exceed DB limits; reference the builder usage around
AuditEventEntity.builder() and the entity.getFileName() call when applying these
changes.

Comment on lines +31 to +40
String method = request.getMethod();
boolean isMutation = "POST".equalsIgnoreCase(method) ||
"PUT".equalsIgnoreCase(method) ||
"DELETE".equalsIgnoreCase(method) ||
"PATCH".equalsIgnoreCase(method);

// Only record mutations or errors to reduce noise
if (!isMutation && ex == null) {
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Non-mutating requests that fail without throwing will silently skip audit.

afterCompletion receives ex only for exceptions that propagate out of the handler. Failures handled by @ExceptionHandler/@ControllerAdvice (e.g., AccessDeniedException, AuthenticationException, validation errors mapped to 4xx) typically arrive here with ex == null and a non-2xx response.getStatus(). With the new guard, a GET that is rejected with 401/403/500 via a controller advice will not be audited, which weakens audit coverage for exactly the reads security auditors usually care about.

Consider also auditing non-mutating requests when the response status indicates an error:

Proposed fix
-        // Only record mutations or errors to reduce noise
-        if (!isMutation && ex == null) {
-            return;
-        }
+        // Only record mutations or errors to reduce noise
+        int status = response != null ? response.getStatus() : 0;
+        boolean isError = ex != null || status >= 400;
+        if (!isMutation && !isError) {
+            return;
+        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/web/AuditInterceptor.java`
around lines 31 - 40, The current guard in afterCompletion uses isMutation and
ex to skip auditing non-mutations, which misses non-mutating requests that
failed without throwing; update the condition in
AuditInterceptor.afterCompletion to also consider HTTP response status by
checking HttpServletResponse.getStatus() (e.g., treat status >= 400 as an error)
so that when ex == null but response.getStatus() >= 400 you still proceed with
audit logging; refer to the isMutation boolean, the ex parameter and
response.getStatus() when modifying the early-return logic.

<td th:text="${e.actorRole}">role</td>
<td th:text="${e.actorId}">actor</td>
<td th:text="${e.httpMethod}">GET</td>
<td th:text="${#temporals.format(e.occurredAt, 'yyyy-MM-dd HH:mm:ss')}">time</td>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Formatting occurredAt can NPE and uses JVM default timezone.

#temporals.format(e.occurredAt, 'yyyy-MM-dd HH:mm:ss') will throw a TemplateProcessingException if any row has a null occurredAt, breaking the whole table render. Although AuditService.record backfills occurredAt, the DB/entity permits null and a single legacy row would break the audit page. Also, formatting without a zone uses the JVM default, which is surprising for audit timestamps — prefer UTC or a consistent zone.

Proposed fix
-                <td th:text="${`#temporals.format`(e.occurredAt, 'yyyy-MM-dd HH:mm:ss')}">time</td>
+                <td th:text="${e.occurredAt != null ? `#temporals.formatISO`(e.occurredAt) : ''}">time</td>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<td th:text="${#temporals.format(e.occurredAt, 'yyyy-MM-dd HH:mm:ss')}">time</td>
<td th:text="${e.occurredAt != null ? `#temporals.formatISO`(e.occurredAt) : ''}">time</td>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/templates/audit/list.html` at line 51, Replace the fragile
expression that directly calls `#temporals.format`(e.occurredAt, ...) with a
null-safe, zone-explicit expression in the template cell: guard e.occurredAt
(e.g. use a ternary or th:if) so a null occurredAt renders a safe placeholder
like "—" instead of throwing, and convert/format the timestamp in UTC (for
example by converting to a ZonedDateTime with T(java.time.ZoneId).of('UTC')
before formatting) so the audit time is not rendered using the JVM default zone;
update the expression that references e.occurredAt in list.html accordingly.

…and refine error handling in GlobalControllerAdvice

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main/java/org/example/projektarendehantering/application/service/AuditService.java (1)

68-99: ⚠️ Potential issue | 🟠 Major

Broadcasting inside the transaction can leak phantom audit events on rollback.

broadcast(event) runs inside the @Transactional boundary of record(...) and pushes the DTO to all SSE clients before the transaction commits. If the surrounding transaction rolls back (e.g., a subsequent failure in the calling service), subscribers have already observed an "audit event" that was never persisted — a correctness problem for a live audit stream and potentially misleading from a compliance standpoint. Additionally, the sequential emitter.send(...) calls perform blocking I/O while holding the DB transaction open, so a slow/stuck client can extend transaction duration.

Consider deferring the broadcast to after commit:

🛠️ Proposed fix
     `@Transactional`
     public void record(AuditEventEntity event) {
         if (event == null) return;
         if (event.getId() == null) {
             event.setId(UUID.randomUUID());
         }
         if (event.getOccurredAt() == null) {
             event.setOccurredAt(Instant.now());
         }
         event.setQueryString(sanitizeAuditPayload(event.getQueryString()));
-        auditEventRepository.save(event);
-
-        broadcast(event);
+        AuditEventEntity saved = auditEventRepository.save(event);
+
+        if (TransactionSynchronizationManager.isSynchronizationActive()) {
+            TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
+                `@Override`
+                public void afterCommit() {
+                    broadcast(saved);
+                }
+            });
+        } else {
+            broadcast(saved);
+        }
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/application/service/AuditService.java`
around lines 68 - 99, The broadcast is executed inside the `@Transactional`
record(AuditEventEntity) method which can leak phantom events and perform
blocking I/O while the DB transaction is open; refactor so persistence happens
in record(...) but broadcasting runs after commit (and off the transaction
thread). Specifically: keep auditEventRepository.save(event) in
record(AuditEventEntity) and remove the direct call to broadcast(event); instead
publish an application event or register a TransactionSynchronization that calls
broadcast(AuditEventEntity) afterCommit (or use
`@TransactionalEventListener`(phase = AFTER_COMMIT)) so broadcast(...) and its
SseEmitter.send(...) calls run only after the transaction commits and off the DB
transaction context. Ensure broadcast(AuditEventEntity) still handles
IOException/IllegalStateException and emitter cleanup as before.
🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/application/service/AuditService.java (1)

118-129: Returning a pre-failed emitter back to Spring MVC.

If the initial "connected" event fails, the emitter is removed from emitters but still returned to the controller, which hands it to Spring MVC. Depending on the failure state, downstream logging may be noisy and the client gets a broken stream without a clear signal. Consider calling emitter.completeWithError(e) and returning early (or letting the IOException propagate) so the client sees a clean failure and Spring doesn't attempt to drive a dead emitter:

🛠️ Proposed fix
         } catch (IOException e) {
             log.error("Failed to send initial SSE event", e);
             emitters.remove(emitter);
+            emitter.completeWithError(e);
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/application/service/AuditService.java`
around lines 118 - 129, The code currently removes the failed SseEmitter
(emitter) from the emitters set but still returns it to the controller; change
the error handling in the block that sends the initial "connected" event so that
on IOException you call emitter.completeWithError(e) (or rethrow the
IOException) and return early instead of returning the (now-broken) emitter;
update the method that constructs the emitter (the code surrounding emitter,
emitters, and the try/catch that sends the "connected" event in AuditService) to
ensure the failed emitter is not returned to Spring MVC and that the exception
is propagated or an explicit null/clean failure is returned.
src/main/resources/templates/fragments/header.html (1)

44-84: Stream is opened for managers on every page regardless of endpoint access.

Every non-/ui/audit page load for a manager opens a long-lived SSE connection to /api/audit/stream. Combined with the server side's single global emitter list and 30‑minute timeout, this can accumulate one connection per open tab per manager, multiplying server resources needed (threads/connections) and audit broadcast work. Consider opening the stream only on a dedicated page (or a shared worker), or closing it on visibilitychange/pagehide to bound resource usage.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/templates/fragments/header.html` around lines 44 - 84, The
SSE is being opened on every page because the current pathname check is
inverted; change the guard to only create EventSource when on the audit page
(replace if (window.location.pathname === '/ui/audit') return; with if
(window.location.pathname !== '/ui/audit') return;), and add lifecycle cleanup:
after creating const eventSource = new EventSource('/api/audit/stream');
register document.addEventListener('visibilitychange', ...) and
window.addEventListener('pagehide', ...) handlers that call eventSource.close(),
and also ensure eventSource.onerror continues to close on terminal states;
reference the eventSource variable, the '/api/audit/stream' URL, and the
'auditLink' click handler for coordination.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 107-112: Split the combined handler in GlobalControllerAdvice into
two: keep a `@ExceptionHandler`(NoResourceFoundException.class) method (e.g.,
handleNotFound) annotated with `@ResponseStatus`(HttpStatus.NOT_FOUND) that
returns void and does not log, and move AsyncRequestNotUsableException to its
own `@ExceptionHandler`(AsyncRequestNotUsableException.class) method (e.g.,
handleAsyncDisconnect) that does NOT set a 404 status but logs at debug level
(use the logger) and returns void; remove AsyncRequestNotUsableException from
the existing handleSilentExceptions signature and ensure the new async handler
does not change the HTTP status.

In `@src/main/resources/templates/audit/list.html`:
- Around line 83-84: The SSE client code creates timeStr from data.occurredAt
using new Date(...).toISOString() (UTC), while server-rendered rows use
`#temporals.format`(...) with the JVM default zone, causing inconsistent
timestamps; make both use UTC: update the server-side formatting call (the
`#temporals.format`(...) invocation that formats occurredAt) to specify
ZoneId.of("UTC") so it renders yyyy-MM-dd HH:mm:ss in UTC, and keep the
client-side creation of timeStr from data.occurredAt (new
Date(...).toISOString().replace('T',' ').substring(0,19)); also apply the same
ZoneId.of("UTC") change to the other server-side occurrence referenced (the
second `#temporals.format` at the other location).
- Around line 64-78: The event stream handler currently double-reconnects and
can crash on bad JSON; update the onerror/connect logic to stop unconditionally
scheduling connect(): in the eventSource.onerror handler check the instance's
readyState and only call setTimeout(connect, 5000) when eventSource.readyState
=== EventSource.CLOSED (or remove the timer and rely on EventSource
auto-reconnect and only call connect() when readyState === CLOSED), and ensure
connect() still closes the previous eventSource reference before creating a new
one; wrap the JSON.parse(e.data) inside a try/catch in the 'audit-event'
listener (log and return on parse errors) to avoid aborting the handler; and
instead of calling location.reload() when document.getElementById('audit-table')
is missing, create a minimal table shell in the DOM (header/ tbody) so incoming
events can append rows without repeatedly reloading the page—refer to
eventSource, connect(), the 'audit-event' listener, JSON.parse(e.data), and
document.getElementById('audit-table') to locate the changes.
- Around line 85-108: The code injects untrusted fields into the DOM via
row.innerHTML (notably data.description, data.requestPath, data.errorType,
data.actorRole, data.actorId, data.caseId, data.eventName, data.statusChange,
data.responseStatus), causing XSS; fix by replacing the innerHTML build with
DOM-safe construction: create table cells and child elements via
document.createElement and set their textContent (or use a proper HTML-escaping
helper) for every interpolated value, only set attributes like href using an
encoded caseId (use encodeURIComponent(data.caseId) when building the link) and
avoid inserting any raw HTML from data.* fields; ensure conditional
badges/status are appended as text nodes or elements rather than string
fragments so no unescaped input is ever assigned to innerHTML.

In `@src/main/resources/templates/fragments/header.html`:
- Around line 54-58: The onerror handler currently calls eventSource.close()
when readyState is CLOSED or CONNECTING, which prevents built-in reconnects;
update the eventSource.onerror implementation to avoid closing during
EventSource.CONNECTING — only call eventSource.close() when
eventSource.readyState === EventSource.CLOSED (or remove the close call
entirely), keeping the function name eventSource.onerror and the use of
eventSource.close and EventSource.CONNECTING/EventSource.CLOSED to locate and
modify the logic.

---

Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/AuditService.java`:
- Around line 68-99: The broadcast is executed inside the `@Transactional`
record(AuditEventEntity) method which can leak phantom events and perform
blocking I/O while the DB transaction is open; refactor so persistence happens
in record(...) but broadcasting runs after commit (and off the transaction
thread). Specifically: keep auditEventRepository.save(event) in
record(AuditEventEntity) and remove the direct call to broadcast(event); instead
publish an application event or register a TransactionSynchronization that calls
broadcast(AuditEventEntity) afterCommit (or use
`@TransactionalEventListener`(phase = AFTER_COMMIT)) so broadcast(...) and its
SseEmitter.send(...) calls run only after the transaction commits and off the DB
transaction context. Ensure broadcast(AuditEventEntity) still handles
IOException/IllegalStateException and emitter cleanup as before.

---

Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/AuditService.java`:
- Around line 118-129: The code currently removes the failed SseEmitter
(emitter) from the emitters set but still returns it to the controller; change
the error handling in the block that sends the initial "connected" event so that
on IOException you call emitter.completeWithError(e) (or rethrow the
IOException) and return early instead of returning the (now-broken) emitter;
update the method that constructs the emitter (the code surrounding emitter,
emitters, and the try/catch that sends the "connected" event in AuditService) to
ensure the failed emitter is not returned to Spring MVC and that the exception
is propagated or an explicit null/clean failure is returned.

In `@src/main/resources/templates/fragments/header.html`:
- Around line 44-84: The SSE is being opened on every page because the current
pathname check is inverted; change the guard to only create EventSource when on
the audit page (replace if (window.location.pathname === '/ui/audit') return;
with if (window.location.pathname !== '/ui/audit') return;), and add lifecycle
cleanup: after creating const eventSource = new
EventSource('/api/audit/stream'); register
document.addEventListener('visibilitychange', ...) and
window.addEventListener('pagehide', ...) handlers that call eventSource.close(),
and also ensure eventSource.onerror continues to close on terminal states;
reference the eventSource variable, the '/api/audit/stream' URL, and the
'auditLink' click handler for coordination.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0f72f960-b1ba-4997-8425-b7cf935adacb

📥 Commits

Reviewing files that changed from the base of the PR and between e5c85bb and 484bc31.

📒 Files selected for processing (5)
  • src/main/java/org/example/projektarendehantering/application/service/AuditService.java
  • src/main/java/org/example/projektarendehantering/presentation/rest/AuditController.java
  • src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java
  • src/main/resources/templates/audit/list.html
  • src/main/resources/templates/fragments/header.html

Comment thread src/main/resources/templates/audit/list.html
Comment thread src/main/resources/templates/audit/list.html
Comment thread src/main/resources/templates/audit/list.html Outdated
Comment thread src/main/resources/templates/fragments/header.html

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.java (1)

22-22: Consider scoping the CSRF eager-init interceptor to the same paths as auditing.

csrfEagerInitInterceptor is registered without any addPathPatterns(...)/excludePathPatterns(...), so it runs for every request including /static/**, /app.css, /app.js, /error**, and /login**. The work it does is cheap, but it's inconsistent with the narrower scope applied to auditInterceptor and means the CSRF token is eagerly generated on requests that don't need it (e.g., static asset fetches).

If the motivation is to ensure the token cookie is set on pages that will later make mutating API calls, scoping this to /ui/** and /api/** (mirroring the audit interceptor) would be cleaner and avoid unnecessary work on static/error/login paths.

♻️ Proposed scoping
-        registry.addInterceptor(csrfEagerInitInterceptor);
+        registry.addInterceptor(csrfEagerInitInterceptor)
+                .addPathPatterns("/ui/**", "/api/**")
+                .excludePathPatterns(
+                        "/static/**",
+                        "/app.css",
+                        "/app.js",
+                        "/error**",
+                        "/login**"
+                );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.java`
at line 22, The csrfEagerInitInterceptor is registered globally via
registry.addInterceptor(csrfEagerInitInterceptor) causing it to run on all
requests (static, error, login); scope it the same as auditInterceptor by
registering with path patterns (e.g., addPathPatterns("/ui/**", "/api/**")) or
the existing patterns used for auditInterceptor inside AuditWebMvcConfig so the
CSRF eager-init only runs for the UI/API paths that need it and avoids
static/error/login requests.
src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java (1)

58-63: Consider using a RequestMatcher that respects context path.

request.getRequestURI() includes the servlet context path, so if the application is ever deployed under a non-root context (e.g., /app), requests to /app/api/... won't match and will fall through to the default entry point (login redirect). Using Spring's AntPathRequestMatcher / PathPatternRequestMatcher would handle this correctly and also be consistent with the authorizeHttpRequests matchers above.

♻️ Suggested refactor
-            .exceptionHandling(ex -> ex
-                .defaultAuthenticationEntryPointFor(
-                    new HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED),
-                    request -> request.getRequestURI().startsWith("/api/")
-                )
-            )
+            .exceptionHandling(ex -> ex
+                .defaultAuthenticationEntryPointFor(
+                    new HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED),
+                    new AntPathRequestMatcher("/api/**")
+                )
+            )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java`
around lines 58 - 63, The exception handling uses a lambda with
request.getRequestURI() which ignores servlet context path; update
SecurityConfig's exceptionHandling/defaultAuthenticationEntryPointFor to use a
RequestMatcher that respects context path (e.g., new
AntPathRequestMatcher("/api/**") or PathPatternRequestMatcher("/api/**"))
instead of the request -> request.getRequestURI().startsWith("/api/") predicate
so the HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED) is applied correctly when
the app is deployed under a non-root context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java`:
- Around line 220-224: The test method name
getClosedCases_shouldReturnRedirect_whenNotLoggedIn is inconsistent with its
assertion; rename the method to reflect the expected Unauthorized status (e.g.,
getClosedCases_shouldReturnUnauthorized_whenNotLoggedIn) so it matches the other
updated tests in CaseControllerTest and improves readability; update the method
name only (keep the test body and annotations as-is) to the new identifier in
the CaseControllerTest class.

---

Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.java`:
- Line 22: The csrfEagerInitInterceptor is registered globally via
registry.addInterceptor(csrfEagerInitInterceptor) causing it to run on all
requests (static, error, login); scope it the same as auditInterceptor by
registering with path patterns (e.g., addPathPatterns("/ui/**", "/api/**")) or
the existing patterns used for auditInterceptor inside AuditWebMvcConfig so the
CSRF eager-init only runs for the UI/API paths that need it and avoids
static/error/login requests.

In
`@src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java`:
- Around line 58-63: The exception handling uses a lambda with
request.getRequestURI() which ignores servlet context path; update
SecurityConfig's exceptionHandling/defaultAuthenticationEntryPointFor to use a
RequestMatcher that respects context path (e.g., new
AntPathRequestMatcher("/api/**") or PathPatternRequestMatcher("/api/**"))
instead of the request -> request.getRequestURI().startsWith("/api/") predicate
so the HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED) is applied correctly when
the app is deployed under a non-root context.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b1ba43aa-7f59-41d3-9518-627084aa4749

📥 Commits

Reviewing files that changed from the base of the PR and between 484bc31 and b51596b.

📒 Files selected for processing (5)
  • src/main/java/org/example/projektarendehantering/infrastructure/config/AuditWebMvcConfig.java
  • src/main/java/org/example/projektarendehantering/infrastructure/config/SecurityConfig.java
  • src/main/java/org/example/projektarendehantering/infrastructure/web/CsrfEagerInitInterceptor.java
  • src/test/java/org/example/projektarendehantering/presentation/rest/AuditControllerTest.java
  • src/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/test/java/org/example/projektarendehantering/presentation/rest/AuditControllerTest.java

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java (1)

117-138: Consider surfacing validation error details.

For MethodArgumentNotValidException, the binding result (field errors, rejected values) is discarded and replaced with a generic "Validation failed" message. API clients typically need the per-field details to render actionable feedback. Consider extracting e.getBindingResult().getFieldErrors() into the response body for REST requests.

♻️ Suggested enhancement
     `@ExceptionHandler`({HttpMessageNotReadableException.class, MethodArgumentNotValidException.class})
     public Object handleBadRequestException(Exception e, HttpServletRequest request, HttpServletResponse response, Model model) {
         HttpStatus status = HttpStatus.BAD_REQUEST;
         String message = "Invalid request payload";
-        if (e instanceof MethodArgumentNotValidException) {
+        Map<String, String> fieldErrors = Map.of();
+        if (e instanceof MethodArgumentNotValidException manv) {
             message = "Validation failed";
+            fieldErrors = manv.getBindingResult().getFieldErrors().stream()
+                    .collect(java.util.stream.Collectors.toMap(
+                            org.springframework.validation.FieldError::getField,
+                            fe -> fe.getDefaultMessage() != null ? fe.getDefaultMessage() : "invalid",
+                            (a, b) -> a));
         }
 
         if (isRestRequest(request)) {
             return ResponseEntity.status(status)
                     .body(Map.of(
                             "errorCode", "BAD_REQUEST",
                             "message", message,
-                            "status", status.value()
+                            "status", status.value(),
+                            "errors", fieldErrors
                     ));
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`
around lines 117 - 138, The handler handleBadRequestException currently returns
a generic "Validation failed" for MethodArgumentNotValidException; modify it to
extract field details from
((MethodArgumentNotValidException)e).getBindingResult().getFieldErrors() and
include a structured "errors" array (field, rejectedValue, defaultMessage) in
the REST ResponseEntity body (when isRestRequest(request) is true), while
preserving the existing errorCode/message/status; keep non-REST behavior
unchanged. Use the class/field names MethodArgumentNotValidException,
getBindingResult(), getFieldErrors(), isRestRequest(request), and ResponseEntity
to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/AuditService.java`:
- Around line 82-87: afterCommit() currently calls broadcast(event) directly
which allows exceptions (e.g., from auditEventMapper.toDTO or SSE emitter.send)
to propagate after DB commit; wrap the entire call to broadcast(event) inside a
try-catch in the TransactionSynchronization registered in
TransactionSynchronizationManager.registerSynchronization (i.e., in the
afterCommit override) and log any Throwable with context so SSE/mapper failures
do not affect the committed request response.

In `@src/main/resources/templates/audit/list.html`:
- Around line 71-168: The SSE handler
(eventSource.addEventListener('audit-event', ...)) currently prepends every
incoming row without checking the active filters; update the handler to read the
current filter inputs (e.g., from/to date inputs, caseId, and size/pageSize
controls used to populate the initial table) and apply them before inserting:
parse data.occurredAt and ignore events outside the from/to window, ignore when
data.caseId doesn't match the active caseId filter (if set), and ignore events
that don't match other active filters (status/actor/event if present). Also
enforce the current page size after inserting by checking
table.querySelectorAll('tbody tr').length and trimming excess rows from the end
so the streamed updates never make the table exceed the selected
size/pagination. Use the existing symbols (data, occurredAt, caseId, tbody, row,
and the SSE handler registered via eventSource.addEventListener('audit-event'))
to locate where to add these checks and trimming logic.
- Around line 37-54: The page currently always instantiates an EventSource to
'/api/audit/stream' (see connect(), eventSource and element
'connection-status'), causing repeating 403 reconnects for non-manager users;
change the template to include a server-side boolean (e.g., canOpenAuditStream
or isManager) and gate the client-side stream logic so connect() / new
EventSource('/api/audit/stream') only runs when that flag is true, otherwise
update 'connection-status' to show streaming is disabled/unauthorized and do not
call EventSource; ensure the server-side variable is populated from the same
permission check that protects the '/api/audit/stream' endpoint so UI and
backend permissions stay consistent.

---

Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java`:
- Around line 117-138: The handler handleBadRequestException currently returns a
generic "Validation failed" for MethodArgumentNotValidException; modify it to
extract field details from
((MethodArgumentNotValidException)e).getBindingResult().getFieldErrors() and
include a structured "errors" array (field, rejectedValue, defaultMessage) in
the REST ResponseEntity body (when isRestRequest(request) is true), while
preserving the existing errorCode/message/status; keep non-REST behavior
unchanged. Use the class/field names MethodArgumentNotValidException,
getBindingResult(), getFieldErrors(), isRestRequest(request), and ResponseEntity
to locate and implement the change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7b752dc6-e77b-44f2-a715-6fa67d3ef125

📥 Commits

Reviewing files that changed from the base of the PR and between b51596b and 41d8a44.

📒 Files selected for processing (5)
  • src/main/java/org/example/projektarendehantering/application/service/AuditService.java
  • src/main/java/org/example/projektarendehantering/presentation/web/GlobalControllerAdvice.java
  • src/main/resources/templates/audit/list.html
  • src/main/resources/templates/fragments/header.html
  • src/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java

Comment on lines +37 to +54
<div style="display:flex; justify-content: space-between; align-items:center; margin-bottom: 12px;">
<h2 style="margin:0;">Audit Log</h2>
<div id="connection-status" class="muted" style="font-size: 0.8em;">
<span style="color: var(--danger-color);">●</span> Real-time: Disconnected
</div>
</div>

<script th:inline="javascript">
(function() {
const statusEl = document.getElementById('connection-status');
if (!statusEl) return;

let eventSource = null;

function connect() {
if (eventSource) eventSource.close();

eventSource = new EventSource('/api/audit/stream');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "*.java" | xargs rg -l "audit" -i | head -20

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 1655


🏁 Script executed:

fd -type f -name "*Audit*.java" -o -name "*audit*.java"

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 257


🏁 Script executed:

# Search for the audit controller and endpoints
rg -i "class.*audit.*controller" --type java -A 5

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 2220


🏁 Script executed:

# Search for /api/audit/stream endpoint
rg "/api/audit/stream|audit/stream" --type java

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 69


🏁 Script executed:

# Search for /ui/audit endpoint
rg "/ui/audit" --type java

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 204


🏁 Script executed:

cat -n src/main/java/org/example/projektarendehantering/presentation/web/AuditUiController.java

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 2637


🏁 Script executed:

cat -n src/main/java/org/example/projektarendehantering/presentation/rest/AuditController.java

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 3070


🏁 Script executed:

cat -n src/main/java/org/example/projektarendehantering/application/service/AuditService.java

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 14931


🏁 Script executed:

# Also check the list.html template to see if currentActor is available
cat -n src/main/resources/templates/audit/list.html | head -80

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 4163


🏁 Script executed:

# Search for currentActor in templates
rg "currentActor" --type html

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 2004


🏁 Script executed:

# Check the header fragment
find . -name "header*" -type f | grep -i fragment | head -5

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 139


🏁 Script executed:

# Also check if there's a configuration that adds currentActor to all models
rg "addAttribute.*currentActor|currentActor.*addAttribute" --type java

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 69


🏁 Script executed:

# Search for where currentActor is added to the model
rg "currentActor" --type java -B 2 -A 2 | head -50

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 796


🏁 Script executed:

# Check if there's an interceptor or config that adds currentActor to all models
rg "HandlerInterceptor|addAttributes|Model.*currentActor" --type java

Repository: ithsjava25/project-backend-code-trauma-team

Length of output: 842


Gate the live stream UI to users allowed to open the stream.

The template unconditionally opens /api/audit/stream, which rejects non-manager users with a 403 error. Since /ui/audit has no access control while the stream endpoint is manager-only, doctors and nurses reaching the page will see repeated reconnection attempts and a "Disconnected (reconnecting...)" status state. Meanwhile, AuditService.listEvents correctly supports their filtered audit access.

Proposed fix
-        <div style="display:flex; justify-content: space-between; align-items:center; margin-bottom: 12px;">
+        <div style="display:flex; justify-content: space-between; align-items:center; margin-bottom: 12px;">
             <h2 style="margin:0;">Audit Log</h2>
-            <div id="connection-status" class="muted" style="font-size: 0.8em;">
+            <div th:if="${currentActor != null and currentActor.role.name() == 'MANAGER'}"
+                 id="connection-status" class="muted" style="font-size: 0.8em;">
                 <span style="color: var(--danger-color);">●</span> Real-time: Disconnected
             </div>
         </div>
 
-        <script th:inline="javascript">
+        <script th:if="${currentActor != null and currentActor.role.name() == 'MANAGER'}" th:inline="javascript">
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/resources/templates/audit/list.html` around lines 37 - 54, The page
currently always instantiates an EventSource to '/api/audit/stream' (see
connect(), eventSource and element 'connection-status'), causing repeating 403
reconnects for non-manager users; change the template to include a server-side
boolean (e.g., canOpenAuditStream or isManager) and gate the client-side stream
logic so connect() / new EventSource('/api/audit/stream') only runs when that
flag is true, otherwise update 'connection-status' to show streaming is
disabled/unauthorized and do not call EventSource; ensure the server-side
variable is populated from the same permission check that protects the
'/api/audit/stream' endpoint so UI and backend permissions stay consistent.

Comment thread src/main/resources/templates/audit/list.html
@mattknatt
mattknatt merged commit 318c675 into main Apr 22, 2026
2 checks passed
@mattknatt
mattknatt deleted the feat/auditcleanup branch April 22, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant